Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the web service implementation #4

Merged
merged 46 commits into from
Oct 24, 2024
Merged

Add the web service implementation #4

merged 46 commits into from
Oct 24, 2024

Conversation

tkoscieln
Copy link
Collaborator

@tkoscieln tkoscieln commented May 14, 2024

This PR aims to introduce the complete service to the codebase. It includes a setup for deployment as well.

@tkoscieln tkoscieln marked this pull request as ready for review May 14, 2024 18:29
@tkoscieln tkoscieln force-pushed the use_fast_api branch 2 times, most recently from e53d9ea to 497dcdb Compare May 14, 2024 18:50
@tkoscieln tkoscieln changed the title Use fast api Add a service implementation May 15, 2024
@tkoscieln tkoscieln requested a review from psss May 15, 2024 13:00
@psss psss requested a review from janhavlin June 25, 2024 08:54
@psss psss requested a review from martinhoyer August 12, 2024 13:45
Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments to the pipeline/metadata files. Will look into the actual api/service asap.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/requirements.txt Outdated Show resolved Hide resolved
@martinhoyer
Copy link
Collaborator

@phagara @inknos @spetrovi FastAPI masters, would you please take a look as well? :)

@martinhoyer
Copy link
Collaborator

@psss I've done some changes locally but am not able to push them here. Could you please check if I have the necessary rights?

src/service.py Outdated Show resolved Hide resolved
@psss
Copy link
Contributor

psss commented Aug 14, 2024

@psss I've done some changes locally but am not able to push them here. Could you please check if I have the necessary rights?

Sure, all should be set now.

@martinhoyer
Copy link
Collaborator

Sorry for the mess with the workflow. Redis / KeyDB is driving me crazy. Trying to swithc to KeyDB due to Redis not having FLOSS license and KeyDB apparently being faster.

@martinhoyer
Copy link
Collaborator

fwiw, I'd love to debug it locally, but docker hub is rate limited :/

src/api.py Outdated
plan_path: str = Query(None, alias="plan-path"),
out_format: str = Query("json", alias="format")
):
# Parameter validations
if (test_url is None and test_name is not None) or (test_url is not None and test_name is None):
return "Invalid arguments!"
Copy link

@janhavlin janhavlin Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid arguments should return 400-something status code. I think 400 is suitable one. See https://fastapi.tiangolo.com/tutorial/handling-errors/.

src/api.py Outdated
from fastapi.params import Query
from pydantic import BaseModel
from starlette.responses import HTMLResponse

from src import service

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be nicer if the python package would be named tmt_web or tmt.web (this one rather not, it could potentially collide with code in https://github.com/teemtee/tmt/). So the imports would look like e.g. from tmt_web import service.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is package name, but rather relative import from path, no? Should we add project metadata?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right, it's a relative import, +1 for project metadata.

Copy link
Contributor

@phagara phagara Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relative imports must begin with a dot (eg. "from .src import service"), so this is a standard module/package import call. Python does add the current working directory to its list of module import lookup paths with the highest priority, so in the end it behaves the same way.

It's still a bad practice though and i'd suggest rearranging the project into what's the de-facto standard python project structure nowadays:

pyproject.toml
README.md
src/
  tmt_web/
    __init__.py
    api.py
    generators/
    ...

where a minimal working pyproject.toml could look something like this:

[build-system]
requires = ["setuptools>=64"]
build-backend = "setuptools.build_meta"

[project]
name = "tmt-web"
description = "TMT web app"
readme = "README.md"
requires-python = ">=3.10"
version = "0.0.1"
dependencies = [
    "tmt~=1.35",
    "fastapi~=0.112",
    "httpx~=0.27",
    "uvicorn~=0.30",
    "celery[redis]~=5.4",
]

[tool.setuptools.packages.find]
where = ["src"]

https://packaging.python.org/en/latest/guides/writing-pyproject-toml/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm already working on it. +1 for standard src/tmt_web/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw are there any conventions about underscore vs dash vs .. ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

project names should prefer dashes, while package/module/file names must use underscores

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in PyPI the names are normalized, e.g. jaraco.context can be accessed as:

and thus similarly there is no effect of what you choose in the dependencies as well. What is important though is making sure the sdist package is normalized to _, which I think only started happening in the recent 1-2 years. I am not sure about setuptools, but definitely going for hatchling build system does that for us.

But indeed usual convention for project.name is to use dashes -.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkoscieln I kinda forgot about this. Let's change use a dash :)

tmt-web.yaml Outdated Show resolved Hide resolved
@martinhoyer martinhoyer force-pushed the use_fast_api branch 3 times, most recently from 688029a to 893b472 Compare August 15, 2024 17:11
seberm and others added 3 commits October 24, 2024 13:10
* Add missing docstrings for return values
* Move the settings to one place
* Fix the typo in gitignore
* Fix the RET505 lint
* Provide service arguments as dictionary
* Add a doctype to HTML template
* Change page default title
* Handle various types of responses when celery is disabled
Copy link
Contributor

@psss psss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All major comments should be addressed and tests are green. Let's continue with further improvements in separate pull requests.

@tkoscieln, thanks a lot for implementing this!

@psss psss self-assigned this Oct 24, 2024
@psss psss merged commit db281e0 into main Oct 24, 2024
2 checks passed
@psss psss changed the title Add a service implementation Add the web service implementation Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants